Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JP-3799: First frame bright #8952

Merged
merged 17 commits into from
Nov 20, 2024
Merged

JP-3799: First frame bright #8952

merged 17 commits into from
Nov 20, 2024

Conversation

karllark
Copy link
Collaborator

@karllark karllark commented Nov 12, 2024 •
edited by melanieclarke
Loading

Resolves JP-3799

This PR modifies the firstframe step to optionally not flag the 1st group (aka frame) as DO_NOT_USE for pixels that have ramps that saturation between the 2nd and 3rd groups. In the case of a pixel having a large signal, it has been found that the slope estimated from the 2nd-1st groups and 3rd-2nd groups is equivalent within 1% indicating that the firstframe effect is small compared to the signal in this specific case. This means that for pixels seeing very bright signal (i.e., saturating between the 2nd and 3rd groups), that the slope can be measured from the 1st and 2nd groups. This provides a slope measurement where previously there was none (i.e., a NaN value).

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.datamodels.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.fits_generator.rst
  • changes/<PR#>.set_telescope_pointing.rst
  • changes/<PR#>.pipeline.rst

stage 1

  • changes/<PR#>.group_scale.rst
  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.emicorr.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.ipc.rst
  • changes/<PR#>.firstframe.rst
  • changes/<PR#>.lastframe.rst
  • changes/<PR#>.reset.rst
  • changes/<PR#>.superbias.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.rscd.rst
  • changes/<PR#>.persistence.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.charge_migration.rst
  • changes/<PR#>.jump.rst
  • changes/<PR#>.clean_flicker_noise.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.gain_scale.rst

stage 2

  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.badpix_selfcal.rst
  • changes/<PR#>.msaflagopen.rst
  • changes/<PR#>.nsclean.rst
  • changes/<PR#>.imprint.rst
  • changes/<PR#>.background.rst
  • changes/<PR#>.extract_2d.rst
  • changes/<PR#>.master_background.rst
  • changes/<PR#>.wavecorr.rst
  • changes/<PR#>.srctype.rst
  • changes/<PR#>.straylight.rst
  • changes/<PR#>.wfss_contam.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.fringe.rst
  • changes/<PR#>.pathloss.rst
  • changes/<PR#>.barshadow.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.pixel_replace.rst
  • changes/<PR#>.resample_spec.rst
  • changes/<PR#>.residual_fringe.rst
  • changes/<PR#>.cube_build.rst
  • changes/<PR#>.extract_1d.rst
  • changes/<PR#>.resample.rst

stage 3

  • changes/<PR#>.assign_mtwcs.rst
  • changes/<PR#>.mrs_imatch.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.exp_to_source.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.tso_photometry.rst
  • changes/<PR#>.stack_refs.rst
  • changes/<PR#>.align_refs.rst
  • changes/<PR#>.klip.rst
  • changes/<PR#>.spectral_leak.rst
  • changes/<PR#>.source_catalog.rst
  • changes/<PR#>.combine_1d.rst
  • changes/<PR#>.ami.rst

other

  • changes/<PR#>.wfs_combine.rst
  • changes/<PR#>.white_light.rst
  • changes/<PR#>.cube_skymatch.rst
  • changes/<PR#>.engdb_tools.rst
  • changes/<PR#>.guider_cds.rst

@karllark karllark requested a review from a team as a code owner November 12, 2024 20:22
@codecov Codecov
Copy link

codecov bot commented Nov 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.53%. Comparing base (5a2d0cd) to head (3401d49).
Report is 19 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8952   +/-   ##
=======================================
  Coverage   64.52%   64.53%           
=======================================
  Files         375      375           
  Lines       38739    38745    +6     
=======================================
+ Hits        24997    25003    +6     
  Misses      13742    13742

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@karllark karllark changed the title First frame bright JP-3799: First frame bright Nov 13, 2024
jemorrison
jemorrison approved these changes Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@jemorrison jemorrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and the test added checks that it is working.

melanieclarke
melanieclarke reviewed Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One quick suggestion for the docs for the new argument.

I'll start regression tests.

docs/jwst/firstframe/arguments.rst Outdated Show resolved Hide resolved
@melanieclarke melanieclarke added this to the Build 11.2 milestone Nov 18, 2024
@melanieclarke
Copy link
Collaborator

melanieclarke commented Nov 18, 2024 •
edited
Loading

Regression tests running here:
https://github.com/spacetelescope/RegressionTests/actions/runs/11898230791

All tests pass.

@drlaw1558
Copy link
Collaborator

It might be useful to expand this to unflag group 1 in the cases where saturation occurs in group 2 as well, not just between groups 2 and 3. This would allow for potential future support of single-group data (e.g., using the suppress_one_group=False approach) without having to disable the first-frame step entirely.

@karllark
Copy link
Collaborator Author

It might be useful to expand this to unflag group 1 in the cases where saturation occurs in group 2 as well, not just between groups 2 and 3. This would allow for potential future support of single-group data (e.g., using the suppress_one_group=False approach) without having to disable the first-frame step entirely.

I agree this is a good idea. I will update the PR to make this change. It is a straightforward change.

melanieclarke
melanieclarke reviewed Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update - I'll re-run regression tests.

It looks like some of the docs are still referring to saturating between 2 and 3 - should those be updated, since group 2 can also now be saturated?

@karllark
Copy link
Collaborator Author

It looks like some of the docs are still referring to saturating between 2 and 3 - should those be updated, since group 2 can also now be saturated?

Good catch. I've (hopefully) updated things in all the right places.

melanieclarke
melanieclarke approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one last place that was referring to saturating between group 2 and 3 - suggestion below.

I reran regression tests at the same link above. There are some unrelated failures, but nothing related to this PR.

I'll approve now from my perspective. @drlaw1558 - are you happy with this one now?

docs/jwst/firstframe/arguments.rst Outdated Show resolved Hide resolved
drlaw1558
drlaw1558 approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@drlaw1558 drlaw1558 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M

@karllark
Copy link
Collaborator Author

I've fixed the last doc issue and pushed the commit. Seems to be taking sometime for github to process this update. Just a FYI.

@melanieclarke
Copy link
Collaborator

@karllark - I still don't see the update to the docs. Can you check that you pushed all the changes up?

@karllark
Copy link
Collaborator Author

I did. At the top of this PR you can see a wheel going around saying there has been an update, just hasn't been ingested somehow yet.

@karllark
Copy link
Collaborator Author

And on my fork/branch the commit is present.

@melanieclarke
Copy link
Collaborator

Okay, thanks. I'll close and reopen and see if that triggers an update.

@melanieclarke melanieclarke reopened this Nov 20, 2024
@melanieclarke
Copy link
Collaborator

All good now! I will merge.

@melanieclarke melanieclarke merged commit ae77d55 into spacetelescope:main Nov 20, 2024
31 checks passed
@stscijgbot-jp stscijgbot-jp mentioned this pull request Nov 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jemorrison jemorrison jemorrison approved these changes

@drlaw1558 drlaw1558 drlaw1558 approved these changes

@melanieclarke melanieclarke melanieclarke approved these changes

Assignees
No one assigned
Labels
documentation firstframe testing
Projects
None yet
Milestone
Build 11.2
Development

Successfully merging this pull request may close these issues.
4 participants
@karllark @melanieclarke @drlaw1558 @jemorrison